GH-36411: [Python] Use scikit-build-core as build backend for PyArrow and get rid of setup.py#49259
GH-36411: [Python] Use scikit-build-core as build backend for PyArrow and get rid of setup.py#49259raulcd wants to merge 27 commits intoapache:mainfrom
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@github-actions crossbow submit -g python |
This comment was marked as outdated.
This comment was marked as outdated.
|
@github-actions crossbow submit wheel-*-cp313-cp313-amd64 |
This comment was marked as outdated.
This comment was marked as outdated.
|
@github-actions crossbow submit wheel-windows-cp313-cp313-amd64 |
|
Revision: 166ff63 Submitted crossbow builds: ursacomputing/crossbow @ actions-f1bbdf4eaa
|
|
@github-actions crossbow submit wheel-windows-cp313-cp313-amd64 |
|
Revision: 36fefd4 Submitted crossbow builds: ursacomputing/crossbow @ actions-bd811d95ea
|
|
@github-actions crossbow submit -g python -g wheel |
This comment was marked as outdated.
This comment was marked as outdated.
|
@github-actions crossbow submit python-sdist |
|
Revision: c518c90 Submitted crossbow builds: ursacomputing/crossbow @ actions-ea249e92ce
|
|
@github-actions crossbow submit python-sdist |
|
Revision: 131e2c0 Submitted crossbow builds: ursacomputing/crossbow @ actions-1c58e78aff
|
|
@github-actions crossbow submit python-sdist |
|
Revision: 30e04be Submitted crossbow builds: ursacomputing/crossbow @ actions-3179f97956
|
|
@github-actions crossbow submit -g python -g wheel |
This comment was marked as outdated.
This comment was marked as outdated.
|
@github-actions crossbow submit wheel-windows-cp313-cp313-amd64 |
|
Revision: 5606749 Submitted crossbow builds: ursacomputing/crossbow @ actions-3c4293e220
|
cdd7629 to
1110dbe
Compare
4d3ac22 to
af1f099
Compare
python/CMakeLists.txt
Outdated
| # as it's the most common build type for users building from source. | ||
| # This is mainly relevant for our Windows wheels, which are built with | ||
| # Visual Studio and thus use a multi-config generator with Release. | ||
| # As a note this is only to populate config_internal.h.cmake. |
There was a problem hiding this comment.
Suggestion as per codex 🤖 :
diff --git i/python/CMakeLists.txt w/python/CMakeLists.txt
index 31ce2f149e..7b572823e7 100644
--- i/python/CMakeLists.txt
+++ w/python/CMakeLists.txt
@@ -359,11 +359,9 @@ else()
# CMAKE_BUILD_TYPE is not set at configure time.
# scikit-build-core does the right thing with cmake.build-type and
# adds the corresponding --config but does not populate CMAKE_BUILD_TYPE
- # for those. On this specific case, we set the default to "RELEASE"
- # as it's the most common build type for users building from source.
- # This is mainly relevant for our Windows wheels, which are built with
- # Visual Studio and thus use a multi-config generator with Release.
- # As a note this is only to populate config_internal.h.cmake.
+ # for those. On this specific case, we set the default to "RELEASE".
+ # The actual build type is injected through target compile definitions
+ # for multi-config generators.
set(UPPERCASE_PYBUILD_TYPE "RELEASE")
endif()
@@ -515,6 +513,9 @@ else()
endif()
target_link_libraries(arrow_python PUBLIC Python3::NumPy)
target_compile_definitions(arrow_python PRIVATE ARROW_PYTHON_EXPORTING)
+if(CMAKE_CONFIGURATION_TYPES)
+ target_compile_definitions(arrow_python PRIVATE PYARROW_BUILD_TYPE="$<CONFIG>")
+endif()
set_target_properties(arrow_python PROPERTIES VERSION "${PYARROW_FULL_SO_VERSION}"
SOVERSION "${PYARROW_SO_VERSION}")
install(TARGETS arrow_python
diff --git i/python/pyarrow/src/arrow/python/config_internal.h.cmake w/python/pyarrow/src/arrow/python/config_internal.h.cmake
index e8a6e78c48..f76edccb69 100644
--- i/python/pyarrow/src/arrow/python/config_internal.h.cmake
+++ w/python/pyarrow/src/arrow/python/config_internal.h.cmake
@@ -15,4 +15,6 @@
// specific language governing permissions and limitations
// under the License.
-#define PYARROW_BUILD_TYPE "@UPPERCASE_PYBUILD_TYPE@"
\ No newline at end of file
+#ifndef PYARROW_BUILD_TYPE
+#define PYARROW_BUILD_TYPE "@UPPERCASE_PYBUILD_TYPE@"
+#endifCo-authored-by: Rok Mihevc <rok@mihevc.org>
47ec006 to
b1bd703
Compare
|
@github-actions crossbow submit wheel-windows-cp313-cp313-amd64 |
|
Revision: e2344c7 Submitted crossbow builds: ursacomputing/crossbow @ actions-0633604802
|
AlenkaF
left a comment
There was a problem hiding this comment.
I think this is great optimization, thank you for the work here!
I got sidetracked and haven't looked at the Cython 3.1 issue in a while but would be great to have --cache option tested/checked as a follow-up.
Also, there is some stubs code in the setup.py that will probably need to be looked at as a follow-up also?
I have a draft proposal on top of this branch for that :) #49453 |
|
@github-actions crossbow submit wheel-windows-cp313-cp313-amd64 |
|
Revision: 12f61d0 Submitted crossbow builds: ursacomputing/crossbow @ actions-b00ec5a79b
|
Rationale for this change
Move our PyArrow build backend from setuptools and a custom setup.py to scikit-build-core which is just build backend for CMake related projects.
What changes are included in this PR?
Move from setuptools to scikit-build-core and remove PyArrow setup.py. Update some of the build requirements and minor fixes.
A custom build backend has been also been created in order to wrap scikit-build-core in order to fix problems on License files for monorepos.
pyproject.toml metadata validation expects license files to exist before exercising the build backend that's why we create symlinks. Our thin build backend will just make those symlinks hard-links in order for license and notice files to contain the contents and be added as part of the sdist.
Remove flags that are not used anymore (were only part of setup.py) and documented and validated how the same flags have to be used now.
Are these changes tested?
Yes all Python CI tests, wheels and sdist are successful.
Are there any user-facing changes?
Yes, users building PyArrow will now require the new build dependencies to exercise the build and depending on the flags used they might require to use the new documented way of using those flags.